-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New data source: azurerm_advisor_recommendations
#6867
Conversation
hi @yupwei68 Thanks for opening this PR. Taking a look through here, whilst the code for this looks good - thinking about use-cases for this functionality, it appears this is something that a user would opt to do interactively (using the Azure CLI) or automatically (via the HTTP API's) - rather than something that's useful in Terraform (since there's no means of a user performing actions based on these recommendations). As such whilst I'd like to thank you for this contribution - since it appears this functionality isn't suited to Terraform I'm going to close this pull request for the moment - but we'd recommend instead using the Azure CLI to run interactive commands like this. Thanks! |
…form-provider-azurerm into wyp-advisor-suppression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @yupwei68, overall this looks good but i have some questions about the schema names i've left inline.
azurerm/internal/services/advisor/data_source_advisor_recommendations.go
Outdated
Show resolved
Hide resolved
}, | ||
}, | ||
|
||
"resource_group_names_filter": azure.SchemaResourceGroupNameSetOptional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and
"resource_group_names_filter": azure.SchemaResourceGroupNameSetOptional(), | |
"filter_by_resource_groups": azure.SchemaResourceGroupNameSetOptional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure its worth creating a schema function for this given its not duplicated anywhere else/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I add a schema function because during the reviewing of this PR, the schema function in resource_group.go
continues to be added. I need to change all the ValidateFunc: validateResourceGroupName
to ValidateFunc: ValidateResourceGroupName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think I shall remove the schema function?
@katbyte , Please continue reviewing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yupwei68! LGTM 👍
This has been released in version 2.12.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.12.0"
}
# ... other configuration ... |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
fix: #6119